Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removed deferred call to Wipe on decoding creds #48

Merged
merged 1 commit into from
Sep 20, 2019
Merged

Conversation

aricart
Copy link
Member

@aricart aricart commented Jul 30, 2019

The client is responsible to call wipe after the contents are processed.

…sible to

call wipe after the contents are processed.
@aricart aricart requested review from sasbury and kozlovic and removed request for sasbury August 2, 2019 21:46
Copy link
Member

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At this point, the NATS Go client is no longer doing a wipe.. so I would recommend either:

  • Change the NATS Go client to do the wipe
  • Do the wipe in ParseDecoratedX() but then caller needs to make copy of passed content if it needs to use it after call to ParseDecoratedX().
    Looping @derekcollison.

@aricart
Copy link
Member Author

aricart commented Aug 2, 2019

Yes the lib is not loading the bytes, so the wipe cannot happen on a parameter, it must happen on the caller.

@kozlovic
Copy link
Member

kozlovic commented Aug 2, 2019

I would agree with that. Was just pointing out that NATS Go may have assumed that it was done in the lib (because it was) and removed the wipe calls, which could be put back. Let's wait for @derekcollison to chime in.

@derekcollison
Copy link
Member

Looking at the way wipe was used in the jwt lib I agree it should not be there. We should add back into the Go client.

Copy link
Member

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@aricart aricart merged commit c2e662a into master Sep 20, 2019
@aricart aricart deleted the fix-clear branch December 3, 2019 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants